fix(llm): raise OutputTooLongError when a non-streaming completion is truncated - #3827
Conversation
Strix Security ReviewWarning This pull request has 44 commits after the last Strix review ( No security issues found. Updated for Reviewed by Strix |
koriyoshi2041
left a comment
koriyoshi2041
left a comment
There was a problem hiding this comment.
The length check needs to happen before the empty-content branch. OpenAI-compatible providers can return content="" with finish_reason="length" when the limit is exhausted before any visible token; this branch currently converts that truncation into a retryable ProviderResponseError, so call() repeats the same request and fact extraction never reaches its OutputTooLongError auto-split path.
I reproduced this at fd11c15 by passing an empty-content/length response to _content_or_error: it raises ProviderResponseError, while the non-empty truncation tests pass. Moving the finish-reason check ahead of the content check and adding that case should keep the recovery contract consistent for both empty and partial truncations. The focused provider suites otherwise pass 12/12 with proxy variables cleared.
|
You are right, and the sentence in the description defending that ordering was wrong. The cost is worse than a class change: the empty-content branch sets Pushed in aeea639, with the Correcting one line of this comment, edited in afterwards: I wrote that both siblings raise ahead of their content read. Only Three tests: the |
koriyoshi2041
left a comment
koriyoshi2041
left a comment
There was a problem hiding this comment.
Rechecked at aeea639. The length guard now runs before the empty-content path, and the new call-level case confirms an empty truncation raises OutputTooLongError without retrying (create.call_count == 1); the non-truncated empty-response control still keeps the existing retryable provider error. The focused truncation suite passes 7/7, and git diff --check HEAD^ HEAD is clean.
|
Flagging that @koriyoshi2041 opened #3854 for the same issue about a day after this one. It makes the same call in the same place: raise One substantive difference between them. #3854 raises before the Happy to close this in favour of #3854 if the smaller diff is easier to take, or to move the guard above the message check here and carry the tests. No preference from me, whichever suits the review. |
… truncated chat.completions.create() reports a token-limit truncation only through finish_reason. It never raises LengthFinishReasonError, so the handler in call() that converts that exception into OutputTooLongError cannot fire for either of the two call sites that use create(), and _content_or_error returned the truncated body as though it were complete. For structured output the caller's json.loads then failed on the cut, which the retry ladder treated as a transient parse error and retried against the same token limit. For free-form output the truncated text was returned with nothing to signal the cut. The fact-extraction auto-split recovers from truncation by catching OutputTooLongError, so it could never fire on this path. Detect the truncation where the signal arrives and raise there, matching the two sibling providers: litellm_llm raises on finish_reason == "length" and openai_responses_llm raises on an incomplete status of max_output_tokens. The check sits after the empty-content branch, so a response that is both empty and truncated keeps its existing ProviderResponseError. Fixes vectorize-io#3811
The finish_reason == "length" check sat after the empty-content branch, so a budget exhausted before the first visible token raised the retryable ProviderResponseError instead. call() retries that class, which re-sends the identical request against the same limit, and _extract_facts_with_auto_split never receives the OutputTooLongError it splits on. Move the check ahead of the content read. Both sibling providers already do this: litellm_llm coerces content to "" before its length check, and openai_responses_llm calls _raise_if_truncated before reading output_text. Two new tests cover the case, and both fail at the previous ordering. The call() one pins create.call_count == 1, which is what the old ordering broke: the run log at that ordering shows four attempts against the same limit before it gave up with the wrong error class. A third test is a control that an empty response with any other finish_reason still raises the retryable ProviderResponseError, so the two paths stay distinct. Reported by koriyoshi2041 in review on vectorize-io#3827.
…ment The comment said both sibling providers raise ahead of their content read. That is true of openai_responses_llm, which calls _raise_if_truncated before reading output_text, and false of litellm_llm: it coerces content to "" at litellm_llm.py:330 and only checks finish_reason at :343, so it reads content first and has no empty-content branch at all. Behaviour is unchanged; this only trims the comment to what the code does.
Follow-up to the truncation fix, from review of vectorize-io#3827. Only `fact_extraction` (auto-split) and `consolidator` (FAIL_FAST) handle `OutputTooLongError`. Raising it for every scope therefore changes what a truncated *free-form* call does: reflect synthesis and mental-model page generation now fail the call instead of returning the cut text as if it were complete. That matches litellm_llm and openai_responses_llm, but it is the opposite of the choice gemini_llm made for the same signal in vectorize-io#3365, where reasoning tokens routinely exhaust the visible budget and a warning was judged better than failing. Neither the raise site nor the tests said so; now both do. Also: document the raise in the `_content_or_error` docstring, annotate the test response factory's return type, and drop a stray blank line. Claude-Session: https://claude.ai/code/session_01DNWFQn8AUN6SApcswHG3aN
1b6e41a to
cbe2e00
Compare
koriyoshi2041
left a comment
koriyoshi2041
left a comment
There was a problem hiding this comment.
The new empty-content case is covered, but the no-message truncation that this branch was meant to pick up is still classified as a retryable provider error. _content_or_error reads message and raises on message is None before it checks finish_reason == "length".
So a choice shaped like message=None, finish_reason="length" still retries the identical request and never reaches the split handler. Please move the length guard ahead of the message is None branch and add that exact regression; the existing empty-string case has a message object and does not cover it.
I rechecked exact head cbe2e0030; the four-commit diff and git diff --check HEAD~4..HEAD are clean. The focused suite could not run in the isolated worktree because the cached environment lacks json_repair, so this finding is from the direct control-flow readback rather than a passing local test receipt.
|
You are right, and the merge did not change it. On main That is the one difference I flagged between this and your #3854 on the 28th: yours puts the guard ahead of the message check, so it covers that shape. Rebased onto main it should reduce to the reorder plus the regression test, and I am happy to leave it with you. If you would rather not, say so and I will open the follow-up. |
) Both parse sites in openai_compatible_llm.py decoded structured output with a bare json.loads and, once the retry budget was spent, gave up. parse_llm_json was never reached, so a response that json_repair could have recovered was dropped after max_retries + 1 full generations against a byte-identical request. Both paths now fall back to parse_llm_json as a last resort, matching what litellm_llm.py has done since #2547/#2544. Repair is gated on a finish_reason that positively reports completion. Repair closes an unterminated string or list by inventing the terminator, so repairing a body that may have been cut turns a loud failure into a short answer reported as a complete one. That is stricter than litellm_llm.py, which repairs whatever it has left; the divergence and its cost are recorded at the gate. The native Ollama path gains the truncation guard the OpenAI-compatible path got in #3827, keyed on done_reason. It sits ahead of the free-form/structured split: a cap landing on a closing brace still parses and still validates, and a cap reached before the first visible token leaves content empty, which the free-form branch reads as a *retryable* ProviderResponseError and re-sends against the same limit (#3811). Free-form calls raise too, matching the sibling path. The native path also stashes provider-reported usage, which it never did. It is recorded before the guards, since a capped call still cost tokens and those are the expensive ones. Fixes #3683. Supersedes #3685 by @chiruu12, which diagnosed the bug and contributed the fix and its tests. Rebased onto #3920, dropping the duplicate finish_reason guard and the three truncation tests that #3827 landed in the meantime, moving the Ollama guard above the free-form branch, and adding coverage for the two free-form cases that move exposes. Co-authored-by: chiruu12 <103719146+chiruu12@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_01DNWFQn8AUN6SApcswHG3aN
3 participants
Root cause
chat.completions.create()reports a token-limit truncation only throughfinish_reason. It never raisesLengthFinishReasonError, which is what theexcept LengthFinishReasonErrorhandler incall()converts intoOutputTooLongError. Both structured-output and free-form calls go throughcreate(), so that handler cannot fire for either of them, and_content_or_errorreturned the truncated body as though it were complete.What that looked like downstream:
json.loadsfailed on the cut, so a truncation surfaced as a JSON parse error. The retry ladder treated it as transient and re-sent a byte-identical request against the same token limit._extract_facts_with_auto_splitrecovers from truncation by catchingOutputTooLongErrorand splitting the chunk, so on this path it could never fire.Fix
Detect the truncation where the signal actually arrives and raise there. This matches the two sibling providers:
litellm_llmraises onfinish_reason == "length", andopenai_responses_llmraises when the response status is incomplete with reasonmax_output_tokens.The check sits ahead of the content read, so a response that is truncated before any visible token raises
OutputTooLongErrortoo. An earlier revision of this PR put it after the empty-content branch and described that as deliberate. That was wrong, and koriyoshi2041 caught it in review: the empty-content branch setsretryable = finish_reason not in {"content_filter"}, solengthcame back retryable andcall()re-sent the same request against the same limit instead of letting the auto-split run.Verification
tests/test_openai_compatible_truncation.py, now 7 tests: 5 fail onmainand all 7 pass on this branch. Two of the 7 are controls that pass on both, one for a normalfinish_reason: "stop"response and one for an empty response with a non-truncation finish reason, which still raises the retryableProviderResponseError.main: thecall()one logs 4 attempts against the same limit and then raisesProviderResponseError.call()tests assertcreate.call_count == 1, pinning that a truncation is not retried against the same limit.test_fact_extraction_retry,test_multi_llm_provider,test_openai_responses_provider,test_deepseek_tool_call_compat,test_consolidation_retry_budget,test_xai_oauth_llmand 8 more) on bothmainand this branch: the set of failures is identical, and all of them are pre-existing intest_xai_oauth_llm.pyonmain.ruff check,ruff format --checkandty check hindsight_apiare clean, with atydiagnostic profile identical tomain.chat.completions.create, so what they pin is this repo's handling of a truncated response shape, not that a given provider emits that shape.test_fact_extraction_retry.py, and the catch site atfact_extraction.py:1907takes the sameOutputTooLongErrorclass this raises, but I did not exercise the whole path in one test.Interaction with #3685
No textual conflict: #3685 edits the
JSONDecodeErrorhandler around line 1053 and this changes_content_or_erroraround line 343. They are complementary rather than competing, but the order matters, so it is worth stating. A truncated response no longer reaches that handler at all, because it raises before returning content. If it did reach it,parse_llm_jsonwould repair the cut into valid but silently partial JSON, whereas raising lets the auto-split re-extract the whole chunk. Non-truncation malformed JSON is untouched by this change and still reaches #3685's repair path.Fixes #3811